Fixes crd per-version validation field path #84560

yue9944882 · 2019-10-30T15:12:21Z

/sig api-machinery
/kind bug
/cc @sttts @liggitt @kubernetes/sig-api-machinery-misc

found this when resolving CI failures for #84005 , i dont think there will be compatiblity issues if we reword it directly?

NONE

...apiextensions-apiserver/pkg/controller/nonstructuralschema/nonstructuralschema_controller.go

liggitt · 2019-10-30T15:40:59Z

@sttts can you verify this will not trigger the dueling CRD condition messages? (this is the exact scenario the generation checking code was intended to protect against, but a verification would be good)

yue9944882 · 2019-10-30T16:10:28Z

currently the protection only works by checking whether the message is identical, which will not be helping the case where there are two apiservers writing different messages/conditions in HA setup:

T0: for two apiservers in a cluster, one SVR1, the other SVR2
T1: the non-structural controllers in both server watched events and triggered reconciliation
T2: SVR1 successfully updated the condition w/ TXT1 and bumped lastSeenGeneration on finish.
T3: SVR2 failed recondition due to update conflict, and will be triggered another retry soon after
T4: SVR2 reconcile again and overwrite the condition to TXT2 and bump its lastSeenGeneration.

the condition will end up twiddling from TXT1 to TXT2, it can be even worse if the replicas are multiplied...

liggitt · 2019-10-30T16:38:52Z

This protects against updating condition more than once for the same generation:

kubernetes/staging/src/k8s.io/apiextensions-apiserver/pkg/controller/nonstructuralschema/nonstructuralschema_controller.go

Lines 140 to 146 in b6bf0d6

    
           // avoid repeated calculation for the same generation 
        
           c.lastSeenGenerationLock.Lock() 
        
           lastSeen, seenBefore := c.lastSeenGeneration[inCustomResourceDefinition.Name] 
        
           c.lastSeenGenerationLock.Unlock() 
        
           if seenBefore && inCustomResourceDefinition.Generation <= lastSeen { 
        
           	return nil 
        
           }

liggitt · 2019-10-30T16:39:16Z

condition is in status and does not modify metadata.generation

lavalamp · 2019-10-30T20:55:27Z

...apiextensions-apiserver/pkg/controller/nonstructuralschema/nonstructuralschema_controller.go

@@ -112,7 +112,7 @@ func calculateCondition(in *apiextensions.CustomResourceDefinition) *apiextensio
 			return cond
 		}

-		pth := field.NewPath("spec", "version").Key(v.Name).Child("schema", "openAPIV3Schema")


Is this an associative list? if so I think keyed by name is at least a somewhat acceptable way to index it. (version->versions seems correct though.)

But how to avoid fighting with a prior apiserver during an upgrade?

I'm not a fan of our current mechanism at all, it seems super brittle. Changes like this one shouldn't be so fraught with danger :/

field path is intended to output the jsonpath to the field, so this should be an index

But how to avoid fighting with a prior apiserver during an upgrade?

we already protected against that, I just wanted a sanity check the mechanism was working

field path is intended to output the jsonpath to the field, so this should be an index

Sounds like something that could (and therefore should) be tested...

yue9944882 · 2019-11-04T06:27:14Z

This protects against updating condition more than once for the same generation:

the protection only works within one apiserver. in an HA setup where the replicas are partially upgraded, e.g. M replicas updating old message and N replicas updating new message, the condition message can be flipping at worst min(M, N) times, isn't it? @liggitt

liggitt · 2019-11-04T19:41:03Z

in an HA setup where the replicas are partially upgraded, e.g. M replicas updating old message and N replicas updating new message, the condition message can be flipping at worst min(M, N) times, isn't it? @liggitt

yes, though the normal HA replicas upgrade scenario which starts with M replicas in steady-state, then does a rolling upgrade to N replicas of the new version would only update the message a single time.

sttts · 2019-11-07T08:07:14Z

@liggitt Why should index based errors cause more flapping than name based? I remember that we talked about flapping in the review, but I don't remember the reasoning for the current code.

Or is it just this change which you fear will cause flapping? Sure it will, for a 3 node HA cluster this will cause max 6 updates of the condition, per CRD. That does not sound like a big deal IMO.

liggitt · 2019-11-07T13:23:00Z

@liggitt Why should index based errors cause more flapping than name based? I remember that we talked about flapping in the review, but I don't remember the reasoning for the current code.

It's not specific to index-based errors, this is just the first message change I'm aware of where we have old apiservers in play, and I wanted a double-check of our flapping mitigation.

Or is it just this change which you fear will cause flapping? Sure it will, for a 3 node HA cluster this will cause max 6 updates of the condition, per CRD. That does not sound like a big deal IMO.

Agree.

yue9944882 · 2019-11-07T13:51:05Z

any more action item required in this pull?

liggitt · 2019-11-07T14:08:43Z

/lgtm
/approve

k8s-ci-robot · 2019-11-07T14:09:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, yue9944882

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~staging/src/k8s.io/apiextensions-apiserver/OWNERS~~ [liggitt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from liggitt and sttts October 30, 2019 15:12

liggitt reviewed Oct 30, 2019

View reviewed changes

...apiextensions-apiserver/pkg/controller/nonstructuralschema/nonstructuralschema_controller.go Outdated Show resolved Hide resolved

yue9944882 force-pushed the bugfix/crd-per-version-validation-field-path branch from bc0a784 to 65d2fe0 Compare October 30, 2019 15:42

fixes crd per-version validation field path

0153473

yue9944882 force-pushed the bugfix/crd-per-version-validation-field-path branch from 65d2fe0 to 0153473 Compare October 30, 2019 15:44

lavalamp reviewed Oct 30, 2019

View reviewed changes

sttts added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Nov 6, 2019

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Nov 6, 2019

k8s-ci-robot assigned liggitt Nov 7, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 7, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 7, 2019

k8s-ci-robot merged commit 9c5952e into kubernetes:master Nov 7, 2019

k8s-ci-robot added this to the v1.17 milestone Nov 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes crd per-version validation field path #84560

Fixes crd per-version validation field path #84560

yue9944882 commented Oct 30, 2019 •

edited

liggitt commented Oct 30, 2019 •

edited

yue9944882 commented Oct 30, 2019 •

edited

liggitt commented Oct 30, 2019

liggitt commented Oct 30, 2019

lavalamp Oct 30, 2019

liggitt Oct 30, 2019 •

edited

lavalamp Oct 30, 2019

yue9944882 commented Nov 4, 2019

liggitt commented Nov 4, 2019

sttts commented Nov 7, 2019 •

edited

liggitt commented Nov 7, 2019

yue9944882 commented Nov 7, 2019

liggitt commented Nov 7, 2019

k8s-ci-robot commented Nov 7, 2019

Fixes crd per-version validation field path #84560

Fixes crd per-version validation field path #84560

Conversation

yue9944882 commented Oct 30, 2019 • edited

liggitt commented Oct 30, 2019 • edited

yue9944882 commented Oct 30, 2019 • edited

liggitt commented Oct 30, 2019

liggitt commented Oct 30, 2019

lavalamp Oct 30, 2019

Choose a reason for hiding this comment

liggitt Oct 30, 2019 • edited

Choose a reason for hiding this comment

lavalamp Oct 30, 2019

Choose a reason for hiding this comment

yue9944882 commented Nov 4, 2019

liggitt commented Nov 4, 2019

sttts commented Nov 7, 2019 • edited

liggitt commented Nov 7, 2019

yue9944882 commented Nov 7, 2019

liggitt commented Nov 7, 2019

k8s-ci-robot commented Nov 7, 2019

yue9944882 commented Oct 30, 2019 •

edited

liggitt commented Oct 30, 2019 •

edited

yue9944882 commented Oct 30, 2019 •

edited

liggitt Oct 30, 2019 •

edited

sttts commented Nov 7, 2019 •

edited